#387 #398

Cia15 · 2023-09-12T12:32:45Z

This is my proposed fix for the issue #387.
Using LibGit2.GitCommit() and LibGit2.message() I added the gitcommit message. However, I haven't written the test for this one. Any feedback would be welcomed. Have a nice day!

Datseris

Why do we need the statistics dependency and the settings.json file?

For the tests: you said you haven't written any tests, but did you try out this PR locally and it did work, i.e, it solved the #387 ?

Datseris · 2023-09-13T16:28:31Z

src/saving_tools.jl

-"""
-    gitpatch(gitpath = projectdir())
-
-Generates a patch describing the changes of a dirty repository
+""" Generates a patch describing the changes of a dirty repository


I am confused about this documentation string change. It is a mistake. The previous version was obviously clearer.

Datseris · 2023-09-13T16:29:04Z

src/saving_tools.jl

@@ -200,6 +199,7 @@ Dict{Symbol,Any} with 3 entries:
 function tag!(d::AbstractDict{K,T};
        gitpath = projectdir(), force = false, source = nothing,
        storepatch::Bool = readenv("DRWATSON_STOREPATCH", false),
+        mssg::Bool = false


mssg is not a clear keyword. PLease name it something that conveys its purpose better just as commit_message

Alright, thank you for your feedback. I already changed the mssg keyword to commit_message

Datseris · 2023-09-13T16:30:02Z

In any case, you need to write tests for this PR, by going into the test folder, and modifying one of the existing tests. Pass the keyword in, the load the file, and then see that it has a commit message as field.

Datseris · 2023-09-13T16:30:22Z

And also, thank you for the contribution!

codecov · 2023-09-13T16:33:33Z

Codecov Report

Merging #398 (486df1a) into main (5667d46) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
+ Coverage   90.57%   90.65%   +0.08%     
==========================================
  Files           8        8              
  Lines         753      760       +7     
==========================================
+ Hits          682      689       +7     
  Misses         71       71

Files	Coverage Δ
src/saving_tools.jl	`82.45% <100.00%> (+1.14%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Cia15 · 2023-09-13T17:03:48Z

Dear George Datseris, For preface I would like to apologize for some of the unclear/weird changes on my part. I was doing the issue for a project for my university module and I have limited knowledge and experience with working with github or fixing issue in general. To address some of your feedback: 1. We don't need statistics dependency and the settings.json file, it was a mistake on my part. When we run it locally, it did work and shows the git commit message. 2. The documentation string change is also my fault on that part. 3. The mssg was chosen because my instructor and I thought it would clash with the message function? However, thank you so much for the input. 4. I will try to add the test. Once again thank you so much for the feedback! Cheers, Cia15 George Datseris ***@***.***> schrieb am Mi., 13. Sept. 2023, 6:27 PM:

…

***@***.**** commented on this pull request. Why do we need the statistics dependency and the settings.json file? For the tests: you said you haven't written any tests, but did you try out this PR locally and it did work, i.e, it solved the #387 <#387> ? — Reply to this email directly, view it on GitHub <#398 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BB46HBZF6TPCMBPSQ23CXWDX2HNHTANCNFSM6AAAAAA4UZA3MI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Datseris · 2023-09-15T16:53:01Z

test/stools_tests.jl

@@ -35,7 +35,7 @@ cpath = _setup_repo(false) # clean
 @test !endswith(gitdescribe(cpath), "-dirty")

 # tag!
-function _test_tag!(d, path, haspatch, DRWATSON_STOREPATCH)
+function _test_tag!(d, path, haspatch, has_message, DRWATSON_STOREPATCH)


Its great that you modified this function to test for the existence of a message. However I see two problems: 1) you never call this function with true as the 4th argument, and hence, the function never actually tests the new code. 2) the function does not test whether the message itself is correct. Only tests that it is not the empty string.

I propose for you to revert the modification of the test_tag! function. Instead, in the codeblock @testset "message_name" begin that you have created below, write a custom, hand coded test that explicitly calls tag! and then it explicitly tests that the tagged dictionary has exact information of the commit message, whatever that message is.

I see, thank you! I will try to implement it

Datseris · 2023-09-15T16:53:13Z

.vscode/settings.json

The settings.json file still exists.

Datseris · 2023-09-15T16:53:33Z

src/saving_tools.jl

-"""
-    gitpatch(gitpath = projectdir())
-
+""" 


Why did this change happen? You should revert it.

Datseris · 2023-09-15T16:53:46Z

src/saving_tools.jl

@@ -128,6 +126,7 @@ function gitpatch(path = projectdir(); try_submodule_diff=true)
        repo = LibGit2.GitRepoExt(path)
        gitpath = LibGit2.path(repo)
        gitdir = joinpath(gitpath,".git")
+        ###gitmessage = LibGit2.message() #Add the gitmessage


This comment does not help, rather, it confuses. You should delete it.

src/saving_tools.jl

Datseris · 2023-09-15T16:54:56Z

src/saving_tools.jl

@@ -208,6 +209,7 @@ function tag!(d::AbstractDict{K,T};
    # Get the appropriate keys
    commitname = keyname(d, :gitcommit)
    patchname = keyname(d, :gitpatch)
+    message_name = keyname(d, :gitmessage) ### added


What information does the ### added comment provide? Does it really help? Remove the comment.

Datseris · 2023-09-15T16:55:08Z

src/saving_tools.jl

+            msg = LibGit2.message(mssgcommit)
+            if (msg !== nothing) && (msg != "")
+                 d[message_name] = msg
+            end ### added 


again, remove ### added.

Datseris

I am sorry it took so long to submit another review. I was overwhelemd with other projects.

I am also sorry to drag this PR more, but the testing is still incorrect and needs to change. I am positive that this will be the last round of review though, I tried to make things explicit!

Datseris · 2023-09-22T11:52:10Z

src/saving_tools.jl

-"""
-    gitpatch(gitpath = projectdir())
+""" 
+gitpatch(gitpath = projectdir())


Incorrect. Restore the 4 spaces of identation, otherwise the documentation string will not register correctly.

Datseris · 2023-09-22T11:53:22Z

src/saving_tools.jl

+stored. You can use [`isdirty`](@ref) to check if a repo is dirty. If the commit message is set to true, 
+then the output will display the commit message. 


When referring to code, don't use english words. Use code, formatted as monotype font. Don't write "If the commit message is ...". Write instead "If the commit_message keyword is set to true, then...".

Datseris · 2023-09-22T11:53:40Z

src/saving_tools.jl

@@ -190,16 +192,18 @@ Dict{Symbol,Int64} with 2 entries:
  :y => 4
  :x => 3

-julia> tag!(d)
+julia> tag!(d, commit_message=true)


Suggested change

julia> tag!(d, commit_message=true)

julia> tag!(d; commit_message=true)

it is better syntax to separate keywords by ;.

Datseris · 2023-09-22T11:54:19Z

test/stools_tests.jl

+# Ensure that the correct git message show up
+d_new = tag!(d, gitpath=dpath, commit_message = true)


This doesn't test anything, see above.

Datseris · 2023-09-22T11:56:10Z

test/stools_tests.jl

+    @testset "message_name" begin
+        path = mktempdir(cleanup=true) # delete path on process exit
+        repo = LibGit2.init(path)
+        LibGit2.commit(repo, "tmp repo commit")
+        message_commit_test = LibGit2.GitCommit(repo, "HEAD")
+        message_name = LibGit2.message(message_commit_test)
+        if (message_name !== nothing) && (message_name != "")
+            print(message_name)
+       end


This test block is wrong unfortunately. You need to test the tag! function. This is what DrWatson provides. We don't care about testing Lib2Git.

Please call the tag! function, with the keyword commit_message to true. Then manually examine the tagged dictionary, and manually write the test

@test d["gitmessage"] == "the mesage you expecet"

Not calling the @test macro means you are not testing anything. Printing is not testing.

Sorry, this might be a dumb question, but will it also work if instead I add the @test d["gitmessage"] == "the mesage you expect" in the _test_tag! function? So something like this? I'm terribly sorry for the beginner mistakes..

Don't alter any of the previous code. It is simply not worth it. Why not just add this new test code block, that just has 5 lines of code, and does the test? Simpler = better.

(Sure, there could be ways to alter the previous code, but why do it though? just make a new @testset codeblock with 4 lines of code that call the tag! function and read the message and call an @test on the read message.)

Somehow it didn't work for me.............

Datseris

Besides the outlined changes, you need to also:

Got to Project.toml and increment the minor (2nd number) version.
Got to CHANGELOG.md and add a new section under the new minor version where you state that you added this new functionality.

Datseris · 2023-09-26T11:32:54Z

src/saving_tools.jl

@@ -167,7 +168,8 @@ the project's gitpath). Do nothing if a key `gitcommit` already exists
 repository is not found. If the git repository is dirty, i.e. there
 are un-commited changes, and `storepatch` is true, then the output of `git diff HEAD` is stored
 in the field `gitpatch`.  Note that patches for binary files are not
-stored. You can use [`isdirty`](@ref) to check if a repo is dirty.
+stored. You can use [`isdirty`](@ref) to check if a repo is dirty. If the `commit message` is set to `true`, 


Suggested change

stored. You can use [`isdirty`](@ref) to check if a repo is dirty. If the `commit message` is set to `true`,

stored. You can use [`isdirty`](@ref) to check if a repo is dirty.

If the keyword `commit_message` is set to `true`,

it is important to be precise in documentation strings.

Datseris · 2023-09-26T11:33:43Z

src/saving_tools.jl

@@ -167,7 +168,8 @@ the project's gitpath). Do nothing if a key `gitcommit` already exists
 repository is not found. If the git repository is dirty, i.e. there
 are un-commited changes, and `storepatch` is true, then the output of `git diff HEAD` is stored
 in the field `gitpatch`.  Note that patches for binary files are not
-stored. You can use [`isdirty`](@ref) to check if a repo is dirty.
+stored. You can use [`isdirty`](@ref) to check if a repo is dirty. If the `commit message` is set to `true`, 
+then the output will display the commit message. 


The sentence "then the output will display the commit message." should be imporved. What is correct is that "the dinctionary d will include an additional field "gitmessage" and will contain the git message associated with the commit.

test/stools_tests.jl

Project.toml

CHANGELOG.md

Cia15 added 5 commits September 1, 2023 16:08

added gitmessage

ab013fb

removed unecessary package I accidently added

0c924f5

Installing packages

c8e9643

testing the gitmssg function

4b7ef1a

changed variable in LibGit.2.GitCommit()

a69f9a6

Datseris reviewed Sep 13, 2023

View reviewed changes

Cia15 added 2 commits September 15, 2023 16:52

Added test for the git commit message?

52a2ba0

Added one more line to the git message test

1bbe585

Datseris reviewed Sep 15, 2023

View reviewed changes

Revise the gitmessage test

664e844

Datseris requested changes Sep 22, 2023

View reviewed changes

Changed some of the test for gitmessage

440b471

Datseris requested changes Sep 26, 2023

View reviewed changes

Cia15 added 2 commits September 27, 2023 16:29

Revision on the commit message test, description

fe299f6

changed back to previous

486df1a

Datseris approved these changes Sep 28, 2023

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

correct version

f659ab7

Datseris merged commit cad2a38 into JuliaDynamics:main Sep 28, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#387 #398

#387 #398

Cia15 commented Sep 12, 2023

Datseris left a comment

Datseris Sep 13, 2023

Datseris Sep 13, 2023

Cia15 Sep 15, 2023

Datseris commented Sep 13, 2023

Datseris commented Sep 13, 2023

codecov bot commented Sep 13, 2023 •

edited

Loading

Cia15 commented Sep 13, 2023 via email

Datseris Sep 15, 2023

Cia15 Sep 15, 2023

Datseris Sep 15, 2023

Datseris Sep 15, 2023

Datseris Sep 15, 2023

Datseris Sep 15, 2023

Datseris Sep 15, 2023

Datseris left a comment

Datseris Sep 22, 2023

Datseris Sep 22, 2023

Datseris Sep 22, 2023

Datseris Sep 22, 2023

Datseris Sep 22, 2023

Cia15 Sep 26, 2023

Datseris Sep 26, 2023

Cia15 Sep 26, 2023

Datseris left a comment

Datseris Sep 26, 2023

Datseris Sep 26, 2023

		stored. You can use [`isdirty`](@ref) to check if a repo is dirty. If the commit message is set to true,
		then the output will display the commit message.

	julia> tag!(d, commit_message=true)
	julia> tag!(d; commit_message=true)

		# Ensure that the correct git message show up
		d_new = tag!(d, gitpath=dpath, commit_message = true)

	stored. You can use [`isdirty`](@ref) to check if a repo is dirty. If the `commit message` is set to `true`,
	stored. You can use [`isdirty`](@ref) to check if a repo is dirty.
	If the keyword `commit_message` is set to `true`,

#387 #398

#387 #398

Conversation

Cia15 commented Sep 12, 2023

Datseris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datseris commented Sep 13, 2023

Datseris commented Sep 13, 2023

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

Cia15 commented Sep 13, 2023 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datseris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datseris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 13, 2023 •

edited

Loading